Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AI chatting functionality #11430

Merged
merged 423 commits into from
Aug 14, 2024
Merged

AI chatting functionality #11430

merged 423 commits into from
Aug 14, 2024

Conversation

InAnYan
Copy link
Collaborator

@InAnYan InAnYan commented Jun 26, 2024

Screenshots of the UI changes

Old chat style:
изображение

New chat style:
image

Summarization:
image

AI preferences:
изображение

TODOs

(Update by koppor)

Mandatory checks

  • Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
  • Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (for UI changes)
  • Checked developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
  • Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

@InAnYan
Copy link
Collaborator Author

InAnYan commented Jun 26, 2024

On my machine, when I run "checkstyleMain" from Gradle commands, I have no such errors. It has successfully generated for me a list of violations

@Siedlerchr
Copy link
Member

Then it seems reviewdog seems to have an outdated version of checkstyle

@Siedlerchr
Copy link
Member

I updated the checkstyle reviewdog action to a newer version so the checkstyle works now again and can parse your code :)

@koppor
Copy link
Member

koppor commented Jun 27, 2024

Copy link
Member

@calixtus calixtus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some quick notes after i have skimmed the changes.

Comment on lines 38 to 41

this.onFailure(e -> {
throw new RuntimeException(e);
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Be aware that the whole indexing architecture will be refactored when the lucene search project is integrated.
This will produce merge conflicts. The less changes in the search package, the better.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, this change can be reverted.

NEVER use new RuntimeException(e);. This tears down the whole application and all data is lost. Is this error really that severe? No, it is not.

@@ -42,6 +42,7 @@

/**
* Indexes the text of PDF files and adds it into the lucene search index.
* Also generates embeddings.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will cause merge conflicts when lucene search is merged.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, the whole change can be reverted.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I clicked the revert button.

Comment on lines 254 to 286

/* AI chat style */
-jr-ai-message-user: -jr-accent;
-jr-ai-message-ai: -jr-accent;

/* MarkdownViewer settings */
-mdfx-font-color: black;
-mdfx-link-color: blue;
-mdfx-border-color-1: #888;

-mdfx-bg-color-1: #ccc;
-mdfx-bg-color-2: #ddd;
-mdfx-bg-color-3: #eee;

-mdfx-bq-color-border: #4488cc; /* the blockquote's border color */
-mdfx-bq-color-background: #0000ff0c; /* the blockquote's background color */
}

.markdown-text {
-fx-font-family: "System";
-fx-font-size: 13;
}

.markdown-italic {
-fx-font-style: italic;
}

.markdown-bold {
-fx-font-weight: bold;
}

.markdown-strikethrough {
-fx-strikethrough: true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to be an artifact after ADR34

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That mean,s these .markdown-* and maybe -mdfx-* items can be removed.

@koppor koppor changed the title AI chatting functionality (the right branch) AI chatting functionality Jul 3, 2024
@koppor
Copy link
Member

koppor commented Jul 4, 2024

The embeddings generation makes my machine unsuable. Please try to use lower priority for the indexing threads:

image


Without embeddings generation:

image

@ThiloteE
Copy link
Member

ThiloteE commented Jul 4, 2024

Use as many threads as there are physical cores. Using LLMs, that's the most performant in my personal tests.

Copy link
Member

@koppor koppor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went half thorugh. Will continue later

General comment:

NEVER use new RuntimeException(e);. This tears down the whole application and all data is lost. Is this error really that severe? No, it is not.

Minr comments:

I reverted changes in files which have no impact on the concrete functionality.

I also reverted level@org.jabref.logic.ai.embeddings.AiIngestor = trace, because this is for development only, not for a distribution of JabRef.


I also made code suggestions. You can collect them and commit them togehter (and not create several commits) - https://docs.github.com/de/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/incorporating-feedback-in-your-pull-request

Comment on lines 254 to 286

/* AI chat style */
-jr-ai-message-user: -jr-accent;
-jr-ai-message-ai: -jr-accent;

/* MarkdownViewer settings */
-mdfx-font-color: black;
-mdfx-link-color: blue;
-mdfx-border-color-1: #888;

-mdfx-bg-color-1: #ccc;
-mdfx-bg-color-2: #ddd;
-mdfx-bg-color-3: #eee;

-mdfx-bq-color-border: #4488cc; /* the blockquote's border color */
-mdfx-bq-color-background: #0000ff0c; /* the blockquote's background color */
}

.markdown-text {
-fx-font-family: "System";
-fx-font-size: 13;
}

.markdown-italic {
-fx-font-style: italic;
}

.markdown-bold {
-fx-font-weight: bold;
}

.markdown-strikethrough {
-fx-strikethrough: true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That mean,s these .markdown-* and maybe -mdfx-* items can be removed.

src/main/java/org/jabref/gui/Dark.css Outdated Show resolved Hide resolved
src/main/java/org/jabref/gui/JabRefGUI.java Outdated Show resolved Hide resolved
@koppor
Copy link
Member

koppor commented Jul 4, 2024

If embedding generation is cancleded, JabRef does not shut down cleanly:

image

Meaning: It does not shut down at all.

@koppor koppor added this to the 6.0-alpha milestone Jul 4, 2024
@koppor koppor mentioned this pull request Jul 4, 2024
7 tasks
@koppor
Copy link
Member

koppor commented Jul 4, 2024

  • If no libraries are opened (e..,g because they were closed), no indexing should happen
    image

@koppor
Copy link
Member

koppor commented Jul 4, 2024

Please also test with a shared SQL database. See https://docs.jabref.org/collaborative-work/sqldatabase for details.

Ensure that autosave to a local .tex file is activated.

i assume that either a check for database location == LOCAL -has to be added -- or another storage path for the chat history used.

@koppor
Copy link
Member

koppor commented Jul 4, 2024

Good thing: seems to count files right.

@Article{test,
  file = {:test.pdf},
}
@Comment{jabref-meta: databaseType:bibtex;}

image

@koppor
Copy link
Member

koppor commented Jul 4, 2024

Bad thing: if test.pdf was made availble using the file system (and automatic relinking by JAbRef), it is not indexed:

image

To reproduce: Use BibTeX from above. Open JabRef. Check indexing. Then copy test.pdf to a folder where JabRef can find the PDF.

Copy link
Member

@koppor koppor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last comments :)

Not big ones, just some code things so that others could understand it more if they were about to maintain it.

I am worried about the shutdown. To get this fixed, I think, we need to add LOGGER.trace statements at org.jabref.gui.LibraryTab#onClosed and other places where a shutdown happens - to see which shutdowns are triggered and which are not happening.

src/main/java/org/jabref/gui/JabRefGUI.java Outdated Show resolved Hide resolved
src/main/java/org/jabref/gui/LibraryTab.java Show resolved Hide resolved
src/main/java/org/jabref/gui/entryeditor/AiChatTab.java Outdated Show resolved Hide resolved
src/main/java/org/jabref/gui/entryeditor/AiChatTab.java Outdated Show resolved Hide resolved
tabs.add(new DeprecatedFieldsTab(databaseContext, libraryTab.getSuggestionProviders(), undoManager, dialogService, preferencesService, stateManager, themeManager, libraryTab.getIndexingTaskManager(), bibEntryTypesManager, taskExecutor, journalAbbreviationRepository));
tabs.add(new OtherFieldsTab(databaseContext, libraryTab.getSuggestionProviders(), undoManager, dialogService, preferencesService, stateManager, themeManager, libraryTab.getIndexingTaskManager(), bibEntryTypesManager, taskExecutor, journalAbbreviationRepository));
tabs.add(new RequiredFieldsTab(databaseContext, libraryTab.getSuggestionProviders(), undoManager, dialogService, preferencesService, stateManager, themeManager, libraryTab.getIndexingTaskManager(), libraryTab.getEmbeddingsGenerationTaskManager(), bibEntryTypesManager, taskExecutor, journalAbbreviationRepository));
tabs.add(new ImportantOptionalFieldsTab(databaseContext, libraryTab.getSuggestionProviders(), undoManager, dialogService, preferencesService, stateManager, themeManager, libraryTab.getIndexingTaskManager(), libraryTab.getEmbeddingsGenerationTaskManager(), bibEntryTypesManager, taskExecutor, journalAbbreviationRepository));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parameter libraryTab.getEmbeddingsGenerationTaskManager( is required for the AiTab only. Remove everywhere else.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it's required for PreviewPanel or PreviewTab (#11430 (comment))

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ImportantOptionalFieldsTab do not have a preivew. Therefore, passing the whole EmbeddingsGenerationTaskManager is strange.

@calixtus A) Pass EmbeddingsGenerationTaskManager down to all tabs or B) use ´Injector.instantiateModelOrService(EmbeddingsGenerationTaskManager.class)` when really needed?

comment to A): This is required, because we have abstract class FieldsEditorTab extends EntryEditorTab implements OffersPreview {. And each preview offers drag'n'drop of files. Thus, file events should be handled.

I see more options (with more work)

  • C): Move implements OffersPreview to the tabs really offering a preview and try to pass the EmbeddingsGenerationTaskManager only to those
  • D) Use "the" event bus to handle file events and have EmbeddingsGenerationTaskManager subscribing to these file changes. This way, we would decouple the places and have "only" org.jabref.gui.externalfiles.ExternalFilesEntryLinker#moveFilesToFileDirRenameAndAddToEntry knowing the task managers - and not somewhere in the UI.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Option D seems very interesting)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Option D seems very interesting)

I like most. But one needs to take care to jump from the JavaFX thread to a Background Thread.

Hints:

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, we did that. Thus. closing this.

@koppor
Copy link
Member

koppor commented Jul 4, 2024

Use as many threads as there are physical cores. Using LLMs, that's the most performant in my personal tests.

Maybe, we need a setting. If the thing takes 1 hour on a "normal" machine to create embeddings and the user cannot work in parallel, this is a no-go in my oppinion. The PC should be usable without any signifcant lags.

I know that Linux is a better in multi tasking. However, there are still persons using Windows. Thus, JabRef should also be useable for Windows users.

Copy link
Member

@koppor koppor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some more comments maybe (foudn another firefox window)

@koppor
Copy link
Member

koppor commented Aug 13, 2024

I refactored API key handling - but org.jabref.gui.menus.FileHistoryMenuTest still has mock issues.

koppor
koppor previously approved these changes Aug 13, 2024
@InAnYan
Copy link
Collaborator Author

InAnYan commented Aug 14, 2024

@koppor #11430 (comment)

what kind of mock issues?

I ran the test, and it seems okay, or am I doing something wrong?

@Siedlerchr
Copy link
Member

yeah seems to be all green now

@koppor
Copy link
Member

koppor commented Aug 14, 2024

@koppor #11430 (comment)

what kind of mock issues?

I ran the test, and it seems okay, or am I doing something wrong?

Revert my commit 8ec3876 and try again. (Do not push the revert)

git revert 8ec3876

@koppor
Copy link
Member

koppor commented Aug 14, 2024

For me, it is OK. More changes can be done in a follow-up PR. We collected issues for follow up at https://github.com/InAnYan/jabref/milestone/12.

@koppor
Copy link
Member

koppor commented Aug 14, 2024

I think, the last commit is OK.

Even if I fear that changing the prompt only will lead to a complete re-indexing, which is not desired at all!

koppor
koppor previously approved these changes Aug 14, 2024
Copy link
Contributor

github-actions bot commented Aug 14, 2024

The build for this PR is no longer available. Please visit https://builds.jabref.org/main/ for the latest build.

@Siedlerchr Siedlerchr added this pull request to the merge queue Aug 14, 2024
Merged via the queue into main with commit 8a0edc2 Aug 14, 2024
27 of 28 checks passed
@Siedlerchr Siedlerchr deleted the ai-pr-1 branch August 14, 2024 16:14
@subhramit
Copy link
Collaborator

Congratulations, Ruslan. This was a big one (for you as well as for JabRef)!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AI Related to AI Chat/Summarization project: GSoC type: feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants